Conversation
src/bitcoin_adaptor.rs
Outdated
| pub fn put_hash_keyed<T>(&mut self, encodable: &T) -> Result<PRef, Box<dyn Error>> | ||
| where T: Serialize + BitcoinHash { | ||
| Ok(self.hammersbald.put_keyed(&encodable.bitcoin_hash()[..], serde_cbor::to_vec(encodable)?.as_slice())?) | ||
| pub fn put_hash_keyed<T>(&mut self, id: &sha256d::Hash, encodable: &T) -> Result<PRef, Box<dyn Error>> |
There was a problem hiding this comment.
Why do you need to change the API here?
There was a problem hiding this comment.
Maybe T could implement an Into<Hash> here?
There was a problem hiding this comment.
My reasoning was that the BitcoinHash trait is being removed and sha256d::Hash is hard coded for get so it makes sense that you can only insert with sha256d::Hash
There was a problem hiding this comment.
One way to keep the same API would be to add the BitcoinHash trait to the chaindb module and implement it on StoredHeader, which is the only type passed to push_hash_keyed. It's hard to know if any other projects are using this lib so I'm sympathetic to not changing the API too much if we don't have to.
| /// Retrieve a bitcoin_object with its hash | ||
| pub fn get_hash_keyed<T>(&self, id: &sha256d::Hash) -> Result<Option<(PRef, T)>, Box<dyn Error>> | ||
| where T: DeserializeOwned + BitcoinHash{ | ||
| where T: DeserializeOwned { |
There was a problem hiding this comment.
Why do we need the BitcoinHash removed here?
There was a problem hiding this comment.
See above, this will be removed in the next version
src/bitcoin_adaptor.rs
Outdated
| bdb.put_hash_keyed(&genesis.bitcoin_hash().as_hash(), &genesis).unwrap(); | ||
| // find it | ||
| if let Some((_, block)) = bdb.get_hash_keyed::<Block>(&genesis.bitcoin_hash()).unwrap() { | ||
| if let Some((_, block)) = bdb.get_hash_keyed::<Block>(&genesis.bitcoin_hash().as_hash()).unwrap() { |
There was a problem hiding this comment.
Maybe use block_hash instead?
There was a problem hiding this comment.
block_hash is only available on master at the moment, got to wait for the next rust-bitcoin release
|
It's still a bit awkward with get_hash_keyed because of the extra generic. The other way is to take a sha256d::Hash but then the caller has to call as_hash(). Probably not worth it to discuss too much as this is only really to get murmel to compile and hammersbald probably won't be used in the future |
|
Changes look good to me. I also confirmed all tests pass: |
dr-orlovsky
left a comment
dr-orlovsky
left a comment
There was a problem hiding this comment.
NACK: BitcoinHash trait was deliberately removed from rust-bitcoin and replaced with normal txid/wtxid() and block_hash functions. I think it will be better to adopt this new API and not just to introduce deprecated trait
|
@jrawsthorne it sounds like your original approach at commit on the 25th with the API change and no BitcoinHash trait is the right way to go. I'll withdraw my suggestion. :-) |
jrawsthorne
force-pushed
the
upgrade-bitcoin
branch
from
e28184f to
45c7ed9
Compare
jrawsthorne
force-pushed
the
upgrade-bitcoin
branch
from
45c7ed9 to
b753e9d
Compare
|
Reverted to the previous state and updated the bitcoin dependency to 0.25 rather than git |
dr-orlovsky
left a comment
dr-orlovsky
left a comment
There was a problem hiding this comment.
utACK b753e9d
My comment on removing hex dev dependency can go into a separate PR
|
|
||
| [dev-dependencies] | ||
| hex = "0.3" | ||
| hex = "0.4" |
There was a problem hiding this comment.
You can get rid of this dependency like it was done for rust-bitcoin (rust-bitcoin/rust-bitcoin@07fb14b) and use Vec::from_hex instead
|
Closed in favor of #14. |
5 participants
This is a breaking change